MGMT-24872: Allow getting OS Images by RHCOS version - #10691
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@giladravid16: This pull request references MGMT-24872 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
size/XXL
openshift-ci
Bot
added
the
api-review
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giladravid16 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10691 +/- ##
==========================================
+ Coverage 44.47% 44.51% +0.04%
==========================================
Files 423 424 +1
Lines 73355 73500 +145
==========================================
+ Hits 32628 32722 +94
- Misses 37809 37838 +29
- Partials 2918 2940 +22
🚀 New features to boost your workflow:
|
WalkthroughChangesThe PR adds centralized OS image resolution, release-based default RHCOS discovery, version-specific lookup APIs, and resolver wiring through InfraEnv controllers, bare-metal inventory, host commands, ignition, and hardware validation. API and CRD documentation now describe OS image versions as RHCOS or OpenShift versions. Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Main
participant InfraEnvController
participant BareMetalInventory
participant OsImageResolver
participant Release
participant OSImages
Main->>OsImageResolver: NewOsImageResolver
Main->>InfraEnvController: inject resolver
Main->>BareMetalInventory: inject resolver
InfraEnvController->>OsImageResolver: GetOsImageForVersion
BareMetalInventory->>OsImageResolver: GetOsImageForInfraEnv
OsImageResolver->>Release: GetDefaultRhcosVersion
OsImageResolver->>OSImages: resolve image by RHCOS or OpenShift version
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/controller/controllers/infraenv_controller.go (1)
699-705: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
context.Background()used instead of a propagated request context.
GetOsImageForInfraEnvis called withcontext.Background()here, while every other call site of the resolver in this diff (download_boot_artifacts_cmd.go,inventory_v2_handlers.go) threads through the real request context. SincesetBootArtifactURLsdoesn't accept acontext.Contextparameter, this call loses cancellation/timeout propagation from the reconcile loop.Consider adding a
ctx context.Contextparameter tosetBootArtifactURLsand passing it through from the caller instead of usingcontext.Background().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/controllers/infraenv_controller.go` around lines 699 - 705, Update setBootArtifactURLs to accept a context.Context parameter and pass that context to OsImageResolver.GetOsImageForInfraEnv instead of context.Background(). Modify its reconcile caller to provide the propagated request context, preserving cancellation and timeout behavior.internal/bminventory/inventory.go (1)
5182-5238: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftExternal OS image/release-image resolution now runs inside DB transactions.
Both
RegisterInfraEnvInternal(transaction opened at line 5182) andUpdateInfraEnvInternal(transaction opened at line 5580) callb.osImageResolver.GetOsImageForVersion(ctx, ...)while the DB transaction is open. Per the PR description, resolving a legacyopenshift_versionnow requires deriving the RHCOS version "from the release image," which implies release-image introspection — a potentially slow, network/exec-bound operation. Previously,OSImageslookups were simple in-memory/config lookups with no such cost. Holding a DB transaction open across this kind of external call risks long-held locks/connections under load.Consider resolving the OS image before opening the transaction (or after committing), passing the resolved result in.
Also applies to: 5658-5667
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/bminventory/inventory.go` around lines 5182 - 5238, Move OS image resolution for legacy openshift_version out of the database transactions in both RegisterInfraEnvInternal and UpdateInfraEnvInternal. Resolve the image before opening the transaction, retain the existing error handling and version derivation, then pass the resolved image or version into the transaction logic instead of calling b.osImageResolver.GetOsImageForVersion while the transaction is active.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/bminventory/inventory.go`:
- Around line 5218-5238: Move the existing b.ValidatePullSecret validation ahead
of the b.osImageResolver.GetOsImageForVersion call in the infraenv creation
flow, validating pullSecret before any external OS-image resolution uses it.
Remove the later duplicate validation near the subsequent processing, while
preserving the existing validation behavior and error handling.
---
Outside diff comments:
In `@internal/bminventory/inventory.go`:
- Around line 5182-5238: Move OS image resolution for legacy openshift_version
out of the database transactions in both RegisterInfraEnvInternal and
UpdateInfraEnvInternal. Resolve the image before opening the transaction, retain
the existing error handling and version derivation, then pass the resolved image
or version into the transaction logic instead of calling
b.osImageResolver.GetOsImageForVersion while the transaction is active.
In `@internal/controller/controllers/infraenv_controller.go`:
- Around line 699-705: Update setBootArtifactURLs to accept a context.Context
parameter and pass that context to OsImageResolver.GetOsImageForInfraEnv instead
of context.Background(). Modify its reconcile caller to provide the propagated
request context, preserving cancellation and timeout behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 49b000c0-024e-489a-b921-7dad7b64993a
⛔ Files ignored due to path filters (10)
api/vendor/github.com/openshift/assisted-service/models/infra_env.gois excluded by!**/vendor/**api/vendor/github.com/openshift/assisted-service/models/infra_env_create_params.gois excluded by!**/vendor/**api/vendor/github.com/openshift/assisted-service/models/infra_env_update_params.gois excluded by!**/vendor/**client/vendor/github.com/openshift/assisted-service/models/infra_env.gois excluded by!**/vendor/**client/vendor/github.com/openshift/assisted-service/models/infra_env_create_params.gois excluded by!**/vendor/**client/vendor/github.com/openshift/assisted-service/models/infra_env_update_params.gois excluded by!**/vendor/**vendor/github.com/openshift/assisted-service/api/v1beta1/infraenv_types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/assisted-service/models/infra_env.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/assisted-service/models/infra_env_create_params.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/assisted-service/models/infra_env_update_params.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (38)
api/v1beta1/infraenv_types.gocmd/main.goconfig/crd/bases/agent-install.openshift.io_infraenvs.yamlconfig/crd/resources.yamldeploy/olm-catalog/manifests/agent-install.openshift.io_infraenvs.yamlinternal/bminventory/inventory.gointernal/bminventory/inventory_test.gointernal/bminventory/inventory_v2_handlers.gointernal/controller/controllers/infraenv_controller.gointernal/controller/controllers/infraenv_controller_test.gointernal/host/hostcommands/download_boot_artifacts_cmd.gointernal/host/hostcommands/download_boot_artifacts_cmd_test.gointernal/host/hostcommands/instruction_manager.gointernal/host/hostcommands/instruction_manager_test.gointernal/imageservice/url_test.gointernal/imageservice/urls.gointernal/oc/mock_release.gointernal/oc/release.gointernal/oc/release_test.gointernal/oc/testdata/coreos-bootimages.yamlinternal/versions/api.gointernal/versions/common.gointernal/versions/common_test.gointernal/versions/kube_api_versions.gointernal/versions/mock_os_image_resolver.gointernal/versions/mock_osimages.gointernal/versions/mock_versions.gointernal/versions/os_image_resolver.gointernal/versions/os_image_resolver_test.gointernal/versions/osimages.gointernal/versions/osimages_test.gointernal/versions/rest_api_versions.gomodels/infra_env.gomodels/infra_env_create_params.gomodels/infra_env_update_params.gorestapi/embedded_spec.gosubsystem/kubeapi/kubeapi_test.goswagger.yaml
💤 Files with no reviewable changes (3)
- internal/versions/mock_versions.go
- internal/versions/kube_api_versions.go
- internal/versions/rest_api_versions.go
| openshiftVersion := params.InfraenvCreateParams.OpenshiftVersion | ||
| pullSecret := swag.StringValue(params.InfraenvCreateParams.PullSecret) | ||
|
|
||
| if b.EnableImageService { | ||
| var osImage *models.OsImage | ||
| osImage, err = b.osImages.GetOsImageOrLatest(params.InfraenvCreateParams.OpenshiftVersion, params.InfraenvCreateParams.CPUArchitecture) | ||
| if openshiftVersion != "" { | ||
| osImage, err = b.osImageResolver.GetOsImageForVersion(ctx, openshiftVersion, params.InfraenvCreateParams.CPUArchitecture, pullSecret) | ||
| if err != nil { | ||
| return common.NewApiError(http.StatusBadRequest, err) | ||
| } | ||
| } else { | ||
| osImage, err = b.osImages.GetLatestOsImage(params.InfraenvCreateParams.CPUArchitecture) | ||
| if err != nil { | ||
| return common.NewApiError(http.StatusBadRequest, err) | ||
| } | ||
| } | ||
| openshiftVersion, err = versions.OsImageVersion(osImage) | ||
| if err != nil { | ||
| return common.NewApiError(http.StatusBadRequest, err) | ||
| } | ||
| openshiftVersion = *osImage.OpenshiftVersion | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Pull secret is used for external OS-image resolution before it's validated.
At line 5219 pullSecret is derived from user input and passed to b.osImageResolver.GetOsImageForVersion(...) at line 5224, but b.ValidatePullSecret(...) doesn't run until line 5317 — after the unvalidated secret has already been used to authenticate an external registry/release-image lookup. Previously OSImages lookups never took a pull secret at all, so this is a new trust-boundary gap introduced by this PR: an unvalidated/malformed pull secret is used before the validation gate that's supposed to guard it.
Consider moving ValidatePullSecret ahead of the OS image resolution call.
🔒️ Suggested reordering
openshiftVersion := params.InfraenvCreateParams.OpenshiftVersion
pullSecret := swag.StringValue(params.InfraenvCreateParams.PullSecret)
+mirroredRegistries := extractMirroredRegistriesFromConfig(log, mirrorRegistryConfiguration)
+if err = b.ValidatePullSecret(mirroredRegistries, pullSecret, ocm.UserNameFromContext(ctx), ""); err != nil {
+ err = errors.Wrap(secretValidationToUserError(err), "pull secret for new infraEnv is invalid")
+ return common.NewApiError(http.StatusBadRequest, err)
+}
if b.EnableImageService {
var osImage *models.OsImage
if openshiftVersion != "" {
osImage, err = b.osImageResolver.GetOsImageForVersion(ctx, openshiftVersion, params.InfraenvCreateParams.CPUArchitecture, pullSecret)
...(remove the now-duplicate ValidatePullSecret call further down)
As per path instructions, **/*.{py,js,ts,go,rs,java,rb,php,kt,swift,cs} requires "Validate at trust boundaries with allow-lists, not deny-lists."
Also applies to: 5316-5321
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/bminventory/inventory.go` around lines 5218 - 5238, Move the
existing b.ValidatePullSecret validation ahead of the
b.osImageResolver.GetOsImageForVersion call in the infraenv creation flow,
validating pullSecret before any external OS-image resolution uses it. Remove
the later duplicate validation near the subsequent processing, while preserving
the existing validation behavior and error handling.
Source: Path instructions
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
The OS Image an infraenv uses is determined by its openshift_version (osImageVersion in the kube-api). So far it was set to an OS Image's openshift_version, but now we'll allow and prioritize it being set to an OS Image's version instead. Users can still set it to an openshift_version which will result in trying to find the RHCOS version for the Release Image corresponding to that openshift_version. If that doesn't work we fallback to matching an OS Image's openshift_version. There are places that require the actual openshift version for an infraenv, which we can't do when the openshift_version is an RHCOS version and there are multiple OS Images with that RHCOS version but different openshift versions. In those cases we will try getting the highest openshift version from those OS Images.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/versions/osimages.go (1)
1-1: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winShared-RHCOS-version selection is non-deterministic and untested.
GetOsImageByRhcosVersionreturns the first matching image viafunk.Findwith no tie-break, unlike the siblingGetHighestOpenshiftVersionForRhcosVersion, and this exact "multiple images share one RHCOS version" scenario is proven to be an expected case (see thesharedRhcosVersionfixtures used for that sibling function's tests) yet is untested forGetOsImageByRhcosVersionitself.
internal/versions/osimages.go#L118-136: makeGetOsImageByRhcosVersionpick deterministically (e.g., highestOpenshiftVersionamong ties, mirroringGetHighestOpenshiftVersionForRhcosVersion) instead of returningfunk.Find's first match.internal/versions/osimages_test.go#L216-243: add a test case with multiple OS images sharing the same RHCOSVersion/architecture to lock in the chosen tie-break behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/versions/osimages.go` at line 1, Update GetOsImageByRhcosVersion to select the matching image with the highest OpenshiftVersion when multiple images share the requested RHCOS Version and architecture, matching GetHighestOpenshiftVersionForRhcosVersion instead of returning funk.Find’s first result. Add a corresponding osimages_test.go case using multiple same-RHCOS-version images to verify the deterministic tie-break.internal/bminventory/inventory_test.go (1)
9223-9252: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a regression test for the multiarch missing-release-image path. Current coverage only covers generic missing-release-image and multiarch success separately, so the combined failure case can regress unnoticed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/bminventory/inventory_test.go` around lines 9223 - 9252, The existing “Create with ClusterID - CPU architecture mismatch” test does not cover the multiarch case where the release image is missing. Add a regression test in the inventory test suite that configures a multiarch cluster or environment, omits the release image, invokes the relevant create/register flow, and asserts the expected failure response and event behavior, reusing the established missing-release-image and multiarch test patterns.
🧹 Nitpick comments (2)
internal/versions/osimages.go (1)
176-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
filterImagesByArchitecturereturns untypedinterface{}instead of[]*models.OsImage.This works only because
funk.Find/funk.Filterfall back to reflection oninterface{}, but it discards compile-time type safety for all three callers. Asserting to[]*models.OsImagehere (as already done insideGetHighestOpenshiftVersionForRhcosVersion) would be clearer and safer.♻️ Proposed typed return
-func (images osImageList) filterImagesByArchitecture(cpuArchitecture string) (interface{}, string, error) { +func (images osImageList) filterImagesByArchitecture(cpuArchitecture string) ([]*models.OsImage, string, error) { cpuArchitecture = common.NormalizeCPUArchitecture(cpuArchitecture) if cpuArchitecture == "" { cpuArchitecture = common.DefaultCPUArchitecture } archImages := funk.Filter(images, func(osImage *models.OsImage) bool { if swag.StringValue(osImage.CPUArchitecture) == "" { return cpuArchitecture == common.DefaultCPUArchitecture } return swag.StringValue(osImage.CPUArchitecture) == cpuArchitecture - }) + }).([]*models.OsImage) if funk.IsEmpty(archImages) { return nil, cpuArchitecture, errors.Errorf("The requested CPU architecture (%s) isn't specified in OS images list", cpuArchitecture) } return archImages, cpuArchitecture, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/versions/osimages.go` around lines 176 - 194, Change filterImagesByArchitecture to return []*models.OsImage instead of interface{}, ensuring its archImages result is typed accordingly while preserving the existing filtering and error behavior. Update all callers to consume the typed slice directly and remove redundant type assertions, including the one in GetHighestOpenshiftVersionForRhcosVersion.internal/versions/osimages_test.go (1)
216-243: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test for shared-RHCOS-version tie-breaking in
GetOsImageByRhcosVersion.Unlike the
GetHighestOpenshiftVersionForRhcosVersionsuite below (245-311), this suite never exercises the case where multiple OS images share the same RHCOSVersion+ architecture. Given that scenario is core to this feature and is explicitly modeled elsewhere in this file, a test here would catch the selection-ambiguity gap noted onosimages.go.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/versions/osimages_test.go` around lines 216 - 243, Add a test in the GetOsImageByRhcosVersion suite covering multiple OS images with the same RHCOS Version and architecture but different OpenShift versions. Assert that the method selects the expected image according to the established tie-breaking behavior, while preserving the existing matching, unsupported-version, and unsupported-architecture tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/versions/osimages.go`:
- Around line 118-136: Update GetOsImageByRhcosVersion to resolve multiple
matching RHCOS entries using the same URL tie-break logic as
GetHighestOpenshiftVersionForRhcosVersion, rather than returning the first
funk.Find result. Preserve the existing architecture filtering and not-found
error behavior.
---
Outside diff comments:
In `@internal/bminventory/inventory_test.go`:
- Around line 9223-9252: The existing “Create with ClusterID - CPU architecture
mismatch” test does not cover the multiarch case where the release image is
missing. Add a regression test in the inventory test suite that configures a
multiarch cluster or environment, omits the release image, invokes the relevant
create/register flow, and asserts the expected failure response and event
behavior, reusing the established missing-release-image and multiarch test
patterns.
In `@internal/versions/osimages.go`:
- Line 1: Update GetOsImageByRhcosVersion to select the matching image with the
highest OpenshiftVersion when multiple images share the requested RHCOS Version
and architecture, matching GetHighestOpenshiftVersionForRhcosVersion instead of
returning funk.Find’s first result. Add a corresponding osimages_test.go case
using multiple same-RHCOS-version images to verify the deterministic tie-break.
---
Nitpick comments:
In `@internal/versions/osimages_test.go`:
- Around line 216-243: Add a test in the GetOsImageByRhcosVersion suite covering
multiple OS images with the same RHCOS Version and architecture but different
OpenShift versions. Assert that the method selects the expected image according
to the established tie-breaking behavior, while preserving the existing
matching, unsupported-version, and unsupported-architecture tests.
In `@internal/versions/osimages.go`:
- Around line 176-194: Change filterImagesByArchitecture to return
[]*models.OsImage instead of interface{}, ensuring its archImages result is
typed accordingly while preserving the existing filtering and error behavior.
Update all callers to consume the typed slice directly and remove redundant type
assertions, including the one in GetHighestOpenshiftVersionForRhcosVersion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 001797de-f495-4ba9-a577-5c302730db71
⛔ Files ignored due to path filters (10)
api/vendor/github.com/openshift/assisted-service/models/infra_env.gois excluded by!**/vendor/**api/vendor/github.com/openshift/assisted-service/models/infra_env_create_params.gois excluded by!**/vendor/**api/vendor/github.com/openshift/assisted-service/models/infra_env_update_params.gois excluded by!**/vendor/**client/vendor/github.com/openshift/assisted-service/models/infra_env.gois excluded by!**/vendor/**client/vendor/github.com/openshift/assisted-service/models/infra_env_create_params.gois excluded by!**/vendor/**client/vendor/github.com/openshift/assisted-service/models/infra_env_update_params.gois excluded by!**/vendor/**vendor/github.com/openshift/assisted-service/api/v1beta1/infraenv_types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/assisted-service/models/infra_env.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/assisted-service/models/infra_env_create_params.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/assisted-service/models/infra_env_update_params.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (46)
api/v1beta1/infraenv_types.gocmd/main.goconfig/crd/bases/agent-install.openshift.io_infraenvs.yamlconfig/crd/resources.yamldeploy/olm-catalog/manifests/agent-install.openshift.io_infraenvs.yamlinternal/bminventory/inventory.gointernal/bminventory/inventory_test.gointernal/bminventory/inventory_v2_handlers.gointernal/controller/controllers/infraenv_controller.gointernal/controller/controllers/infraenv_controller_test.gointernal/hardware/validator.gointernal/hardware/validator_test.gointernal/host/conditions_test.gointernal/host/host_test.gointernal/host/hostcommands/download_boot_artifacts_cmd.gointernal/host/hostcommands/download_boot_artifacts_cmd_test.gointernal/host/hostcommands/instruction_manager.gointernal/host/hostcommands/instruction_manager_test.gointernal/ignition/discovery.gointernal/ignition/discovery_test.gointernal/imageservice/url_test.gointernal/imageservice/urls.gointernal/oc/mock_release.gointernal/oc/release.gointernal/oc/release_test.gointernal/oc/testdata/coreos-bootimages.yamlinternal/versions/api.gointernal/versions/common.gointernal/versions/common_test.gointernal/versions/kube_api_versions.gointernal/versions/mock_os_image_resolver.gointernal/versions/mock_osimages.gointernal/versions/mock_versions.gointernal/versions/os_image_resolver.gointernal/versions/os_image_resolver_test.gointernal/versions/osimages.gointernal/versions/osimages_test.gointernal/versions/rest_api_versions.gomodels/infra_env.gomodels/infra_env_create_params.gomodels/infra_env_update_params.gorestapi/embedded_spec.gosubsystem/disconnected_cluster_test.gosubsystem/kubeapi/kubeapi_test.gosubsystem/subsystem_suite_test.goswagger.yaml
💤 Files with no reviewable changes (3)
- internal/versions/rest_api_versions.go
- internal/versions/kube_api_versions.go
- internal/versions/mock_versions.go
🚧 Files skipped from review as they are similar to previous changes (28)
- config/crd/bases/agent-install.openshift.io_infraenvs.yaml
- api/v1beta1/infraenv_types.go
- deploy/olm-catalog/manifests/agent-install.openshift.io_infraenvs.yaml
- models/infra_env_create_params.go
- internal/versions/api.go
- models/infra_env.go
- subsystem/kubeapi/kubeapi_test.go
- internal/oc/mock_release.go
- models/infra_env_update_params.go
- internal/imageservice/urls.go
- internal/imageservice/url_test.go
- internal/versions/mock_os_image_resolver.go
- internal/host/hostcommands/instruction_manager.go
- internal/host/hostcommands/download_boot_artifacts_cmd_test.go
- internal/versions/os_image_resolver.go
- swagger.yaml
- cmd/main.go
- restapi/embedded_spec.go
- internal/versions/common.go
- internal/bminventory/inventory_v2_handlers.go
- internal/host/hostcommands/download_boot_artifacts_cmd.go
- internal/versions/common_test.go
- internal/host/hostcommands/instruction_manager_test.go
- internal/oc/testdata/coreos-bootimages.yaml
- internal/oc/release.go
- internal/controller/controllers/infraenv_controller_test.go
- internal/bminventory/inventory.go
- internal/controller/controllers/infraenv_controller.go
| // Returns the OsImage entity matching the specified RHCOS version and CPU architecture. | ||
| func (images osImageList) GetOsImageByRhcosVersion(rhcosVersion, cpuArchitecture string) (*models.OsImage, error) { | ||
| archImages, cpuArchitecture, err := images.filterImagesByArchitecture(cpuArchitecture) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| osImage := funk.Find(archImages, func(osImage *models.OsImage) bool { | ||
| return swag.StringValue(osImage.Version) == rhcosVersion | ||
| }) | ||
|
|
||
| if osImage != nil { | ||
| return osImage.(*models.OsImage), nil | ||
| } | ||
|
|
||
| return nil, errors.Errorf( | ||
| "The requested OS image for RHCOS version (%s) and CPU architecture (%s) isn't specified in OS images list", | ||
| rhcosVersion, cpuArchitecture) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
git ls-files | rg '^internal/versions/|osimages|versions'Repository: openshift/assisted-service
Length of output: 7247
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files | rg '^internal/versions/|osimages|versions'Repository: openshift/assisted-service
Length of output: 7247
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map candidate files first
ast-grep outline internal/versions/osimages.go --view expanded
# Find relevant tests and call sites
rg -n "GetOsImageByRhcosVersion|GetHighestOpenshiftVersionForRhcosVersion|GetOsImageForRelease|GetOsImageForVersion" internal -SRepository: openshift/assisted-service
Length of output: 16084
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' internal/versions/osimages.goRepository: openshift/assisted-service
Length of output: 9216
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect tests around the relevant functions
rg -n -A 8 -B 8 "GetHighestOpenshiftVersionForRhcosVersion|GetOsImageByRhcosVersion|shared RHCOS version|OpenshiftVersion" internal/versions -SRepository: openshift/assisted-service
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- files ---'
git ls-files | rg '^internal/versions/|^internal/.*osimages|^.*versions.*\.go$'
echo '--- osimages.go outline ---'
ast-grep outline internal/versions/osimages.go --view expanded
echo '--- osimages.go excerpt ---'
sed -n '1,260p' internal/versions/osimages.go
echo '--- matches ---'
rg -n -A 8 -B 8 "GetOsImageByRhcosVersion|GetHighestOpenshiftVersionForRhcosVersion|GetOsImageForRelease|GetOsImageForVersion" internal/versions -SRepository: openshift/assisted-service
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('internal/versions/osimages.go')
print(p.read_text())
PYRepository: openshift/assisted-service
Length of output: 9572
🌐 Web query:
"GetHighestOpenshiftVersionForRhcosVersion GetOsImageByRhcosVersion osimages.go"
💡 Result:
The functions GetHighestOpenshiftVersionForRhcosVersion and GetOsImageByRhcosVersion are associated with the openshift/assisted-service repository, specifically within its version and OS image management logic [1][2][3]. These utilities are used to map Red Hat Enterprise Linux CoreOS (RHCOS) versions to their compatible OpenShift Container Platform (OCP) release versions [1][4]. Key details regarding these components: 1. Context: These functions help the Assisted Installer (and related services like assisted-image-service) manage the lifecycle of nodes by ensuring the correct OS image is used for a specified OpenShift release [1][5][4]. 2. Functionality: - GetOsImageByRhcosVersion typically retrieves the OS image metadata (such as the URL for the ISO/rootfs and version string) based on a provided RHCOS version or OCP/RHCOS mapping [5][6]. - GetHighestOpenshiftVersionForRhcosVersion is designed to identify the latest or highest compatible OpenShift release version for a given RHCOS base image, which is critical when multiple OCP versions might support the same underlying RHCOS build [1][7]. 3. Implementation: In the assisted-service codebase, these versions are often handled via configurations (such as in AgentServiceConfig or internal hash maps) that map major.minor OpenShift versions to RHCOS image metadata [2][4]. The logic often involves comparing semantic versions to determine the "latest" version, prioritizing stable, non-beta releases [8]. Because these functions are part of internal service logic, they are not typically exposed as public APIs for end-users, but rather operate as backend components within the OpenShift Assisted Installer ecosystem [1][4]. For official version compatibility mapping, Red Hat maintains a dedicated support article that documents the relationship between OpenShift and RHCOS versions [9].
Citations:
- 1: 8b419f6
- 2: 6579cf4
- 3: f129981
- 4: https://github.com/openshift/assisted-service/blob/master/docs/hive-integration/kube-api-select-ocp-versions.md
- 5: https://github.com/openshift/assisted-image-service/
- 6: https://pkg.go.dev/github.com/openshift/assisted-image-service@v0.0.0-20260130223312-503834119b3e
- 7: redhat-best-practices-for-k8s/certsuite@bafffb7
- 8: 0d3e7fb
- 9: https://access.redhat.com/articles/5449111
🌐 Web query:
"\"GetHighestOpenshiftVersionForRhcosVersion\" URL RootFSUrl OsImage"
💡 Result:
There is no public API or function named "GetHighestOpenshiftVersionForRhcosVersion" in the official OpenShift documentation or core repositories. The string appears to be related to internal logic within specific OpenShift components, such as the assisted-service or installer projects, rather than a generally exposed utility [1][2][3]. To determine the OpenShift version corresponding to a specific Red Hat Enterprise Linux CoreOS (RHCOS) version or image, you should rely on the following official methods: 1. Use the OpenShift Installer's built-in stream data: The OpenShift installer maintains metadata (often in JSON format) that maps RHCOS builds to their associated OpenShift releases [4]. You can retrieve stream information using the following command [5]:./openshift-install coreos print-stream-json 2. Query the OpenShift Release metadata: The OpenShift release image contains metadata about the underlying RHCOS version. You can inspect an OCP release image to find the RHCOS version associated with it using the 'oc adm release info' command [6]: oc image info $(oc adm release info --image-for=machine-os-content quay.io/openshift-release-dev/ocp-release:<ocp_version>-) 3. Check Official Compatibility Matrices: Red Hat provides documentation that lists the underlying RHEL minor versions for each OpenShift release [7]. While this does not map exact build identifiers, it identifies the RHEL core version that correlates with specific OpenShift versions [7]. If you are developing custom automation, it is recommended to interact with the OpenShift Update Service (OSUS) or query the ClusterVersion object within a running cluster to determine valid version relationships [8][9]. Avoid relying on undocumented internal helper functions that are subject to change [1][2].
Citations:
- 1: f129981
- 2: 8b419f6
- 3: https://github.com/openshift/installer/blob/master/pkg/asset/rhcos/image.go
- 4: Add new
rhcos-metadatacommand installer#2092 - 5: https://docs.redhat.com/en/documentation/openshift_container_platform/4.12/html/installing_on_a_single_node/install-sno-installing-sno
- 6: https://gist.github.com/orimanabu/e7d400f55529ac98f128e3ecb57ffdca
- 7: https://access.redhat.com/articles/6907891
- 8: https://docs.redhat.com/en/documentation/openshift_container_platform/4.18/html-single/updating_clusters/index
- 9: https://docs.redhat.com/en/documentation/openshift_container_platform/4.15/html-single/updating_clusters/index
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the shared-RHCOS fixture and any URL/RootFSUrl handling in osimages tests
sed -n '240,330p' internal/versions/osimages_test.go
echo '---'
rg -n "RootFSUrl|RootFSURL|URL:|shared-rhcos-version|4.16.2|4.17" internal/versions/osimages_test.go internal/versions -SRepository: openshift/assisted-service
Length of output: 26883
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the OsImage schema fields and where RootFSUrl is used
rg -n "type OsImage struct|RootFSUrl|RootFSURL|RootFS" models internal api vendor -S | head -n 200Repository: openshift/assisted-service
Length of output: 3388
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '240,330p' internal/versions/osimages_test.go
echo '---'
rg -n "RootFSUrl|RootFSURL|URL:|shared-rhcos-version|4.16.2|4.17" internal/versions/osimages_test.go internal/versions -SRepository: openshift/assisted-service
Length of output: 26883
Make the RHCOS lookup deterministic. GetOsImageByRhcosVersion returns the first matching entry, so shared-RHCOS records with different URLs can resolve to the wrong artifact; use the same tie-break as GetHighestOpenshiftVersionForRhcosVersion instead of relying on list order.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/versions/osimages.go` around lines 118 - 136, Update
GetOsImageByRhcosVersion to resolve multiple matching RHCOS entries using the
same URL tie-break logic as GetHighestOpenshiftVersionForRhcosVersion, rather
than returning the first funk.Find result. Preserve the existing architecture
filtering and not-found error behavior.
I asked this in the image-service PR too, but are we sure this is actually a function of the OCP version or could this use the rhcos version instead?
Same question here. What is this checking for and is it something that could depend on RHCOS rather than OCP?
Should we move this check to after the host is bound to a cluster?
This has been a problem for a while and I've wondered if we should just eliminate the ability to to late binding for this case and actually go get the cluster for this. With late binding it has such a large chance to be very wrong, but I don't know how much of a problem that is because I don't understand OKD very well. |
carbonin
left a comment
carbonin
left a comment
There was a problem hiding this comment.
As I'm reading this PR I'm constantly having to check if some openshiftVersion var actually refers to an OCP version or an RHCOS version or possibly optionally both.
Do you think it would be better to add a new column to infraenv in the database and store the RHCOS version there with the intention of eventually deprecating and removing OCP version when all supported releases only use shared RHCOS versions? Because at that point we'll have a field and a bunch of variables that say openshift but are not actually an openshift version. Did we consider that? I don't remember.
| version = *osImage.OpenshiftVersion | ||
| } else { | ||
| return errors.Errorf("OS image entry '%+v' missing OpenshiftVersion field", osImage) | ||
| version, err := versions.OsImageVersion(osImage) |
There was a problem hiding this comment.
I guess the goal here is to make OpenshiftVersion optional, right?
| if err := v.ValidateReleaseImageForRHCOS(ocpVersion, cpuArch); err != nil { | ||
| return err | ||
| } | ||
| if c.CPUArchitecture != common.MultiCPUArchitecture && c.CPUArchitecture != "" && c.CPUArchitecture != cpuArch { |
There was a problem hiding this comment.
Do we still need to normalize these somewhere? IIRC there was some headache around aarch vs arm and x86 vs amd64?
|
|
||
| func (b *bareMetalInventory) getOsImageVersion(ctx context.Context, openshiftVersion, cpuArchitecture, pullSecret string) (string, error) { | ||
| if !b.EnableImageService { | ||
| return openshiftVersion, nil |
There was a problem hiding this comment.
Shouldn't this still do something other than pass the user's value back to them? Previously we were checking which versions were actually configured in the service. What effect will this have it it's wrong?
|
🤦 didn't mean to submit the review yet ... I'll do another |
|
/retest |
|
@giladravid16: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
The OS Image an infraenv uses is determined by its openshift_version (osImageVersion in the kube-api).
So far it was set to an OS Image's openshift_version, but now we'll allow and prioritize it being set to an OS Image's version instead.
Users can still set it to an openshift_version which will result in trying to find the RHCOS version for the Release Image corresponding to that openshift_version.
If that doesn't work we fallback to matching an OS Image's openshift_version.
There are places that require the actual openshift version for an infraenv, which we can't do when the openshift_version is an RHCOS version and there are multiple OS Images with that RHCOS version but different openshift versions.
In those cases we will try getting the highest openshift version from those OS Images.
Those cases include:
List all the issues related to this PR
Closes MGMT-24872
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation